-
Notifications
You must be signed in to change notification settings - Fork 1
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix: create app instead of project on pspace init
#48
base: main
Are you sure you want to change the base?
fix: create app instead of project on pspace init
#48
Conversation
…-endpoint-instead-of-v1projects
…nstead-of-v1projects' of https://github.com/Paperspace/cli into kwang/pla-2431-pspace-init-should-use-v1apps-endpoint-instead-of-v1projects
@@ -127,7 +135,7 @@ export const init = command("init", { | |||
}); | |||
|
|||
if (!flags.json) { | |||
yield `✨ Created app "${app.name}"`; | |||
yield `✨ Created app "${app.config.name}"`; | |||
yield ""; | |||
yield fmt.colors.bold("Console URL"); | |||
yield new URL( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh whoops, this URL is wrong now. Should be
yield new URL(
`/${team}/apps/${app.id}`,
env.get("PAPERSPACE_CONSOLE_URL"),
) + "";
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also I think the message in general is off because calling this command will trigger a deploy. I wonder if that's actually desirable. I mean the alternative is to not actually call apps.create({ config })
here I guess, but then there's no project linking etc.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was also a little surprised by that. I wouldn't expect that initializing a cli would deploy something. But if we remove that then is the command really necessary?
It seems like maybe init
is not the right name, it sounds more like bootstrap
or something?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ya we need to remove the upsert from the init api and just set up project linking. The command is definitely necessary and named correctly 😄 It's similar to docker init
in that it gets you set up with all of the necessary files to start your project. It has the additional step of linking your local directory to a project ID which lets you drop --projectId
flags from commands.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
asserts(res.ok, res); | ||
app = res.data; | ||
} else { | ||
logger.info( | ||
`Project already exists, skipping creation. (link: ${link.id})`, | ||
); | ||
const res = await projects.get({ id: link.id }); | ||
const res = await apps.get({ id: link.id }); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IDK if this line or L133 is actually right anymore either because I think it'd have to be id: app.projectId
. Linking is going to be super wonky now because when you get an app, you're not getting it by project ID (currently) you're getting it by the base deployment UUID.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
might need to make an API change to make this work
I'm actually not clear this is correct because it requires the image and resources specified?